[ACIX-1225] Create -v option on dev env start command
#230
Conversation
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount
branch
from
06f5a68 to
80a4512
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new -v/--volume option to the dev environment start command, allowing users to mount additional directories or Docker volumes into development containers. The feature supports bind mounts from host paths (absolute, relative, or tilde-prefixed) and named Docker volumes, with flags like read-only mode and volume options.
Key Changes:
- Added
extra_mount_specsconfiguration field to store user-provided volume specifications - Implemented
extra_mountsproperty to parse volume specs into Mount objects - Added validation function
__validate_extra_mount_specsto verify mount specifications - Integrated extra mounts into the container creation process in the
start()method
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/dda/env/dev/types/linux_container.py | Implements the core volume mounting functionality including config field, parsing logic, validation, and integration with container start command |
| tests/env/dev/types/test_linux_container.py | Adds comprehensive tests for valid mount scenarios (absolute, relative, multiple, with flags) and invalid mount scenarios (non-existent paths, relative destinations) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount
branch
from
058aba9 to
ea529b7
Compare
This should be interpolated by the shell before it ever gets to Python anyway.
For more complex usecases like using named volumes or weird flags, users should use `--mount`, which will be implemented next.
They would be annoying to fix, and there should not be any Windows-specific behavior here. The positive test cases are still run on Windows.
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount
branch
from
f0409c8 to
07e151f
Compare
| for volume in self.extra_mounts: | ||
| command.extend(("--mount", volume.as_csv())) |
There was a problem hiding this comment.
Unlike most of the other mounts such as for caching, we don't use these in any context other than constructing arguments. What do you think about removing all of the validation logic and having the options extend the arguments directly? This would also support Windows immediately.
There was a problem hiding this comment.
I had sort of the same thought when I noticed that my validation code was becoming way more complex than the actual implementation 😅
My original thought process was that errors coming directly from docker run would be a lot less nice and slightly harder to debug than the ones I've implemented here.
I think your point about Windows paths (great catch btw, I didn't realize splitting on : would break Windows paths... but of course it does 🤦) shifts the balance towards removing it :)
There was a problem hiding this comment.
In the end I only removed the processing for -v, as converting this old-style of volume mounts to Mount objects was a bit kludgy.
I left it for the -m option though, as I think it still makes sense to use the custom class and validation there.
LMK if you want me to remove that for -m as well, although I don't see the value in doing that.
We just pass the arguments straight to `docker run`, with no extra validation, instead. This means we also have to duplicate the test to test separately for `-v` and `-m` options.
…useless validation logic
Co-authored-by: Ofek Lev <[email protected]>
Ishirui
deleted the
pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount
branch
* feat(env): Add ability to mount arbitrary volumes into linux container dev envs * feat(test): Add test for creating dev env with extra bind mounts * refactor(env): Use `Mount` class for specifying extra mounts * feat(env): Add validation logic for `-v` parameter * fix(tests): Fix test after using `Mount` class and adding validation logic * feat(tests): Add test for invalid `-v` params * fix(env): Fix split on colons * fix(env): Disallow passing literal `~` in paths This should be interpolated by the shell before it ever gets to Python anyway. * feat(env): Only allow bind mounts with `-v` For more complex usecases like using named volumes or weird flags, users should use `--mount`, which will be implemented next. * feat(env): Add support for `--mount` parameter * fix(tests): Skip `--mount` invalid param tests on Windows They would be annoying to fix, and there should not be any Windows-specific behavior here. The positive test cases are still run on Windows. * refactor(env): Shorten usage examples for brevity * fix(env): Remove Windows-incompatible processing logic for `-v` We just pass the arguments straight to `docker run`, with no extra validation, instead. This means we also have to duplicate the test to test separately for `-v` and `-m` options. * refactor: Pass `--mount` arguments directly to `docker run` + remove useless validation logic * Final help text cleanup Co-authored-by: Ofek Lev <[email protected]> --------- Co-authored-by: Ofek Lev <[email protected]> d6f0e56
Adds a new option to dev envs:
-v, which allows for mounting arbitrary directories or Docker volumes from the host into the dev env on container creation.